Core/include/Acts/Utilities/BinningType.hpp (1)

50-59: Complete, the JSON serialization mapping is, hmm.

All enum values covered properly, they are. But missing, the tests for JSON serialization are.

Help you create JSON serialization tests, I can. Create a new test file, shall we?

Tests/UnitTests/Examples/Algorithms/Digitization/ModuleClustersTests.cpp (1)

50-53: More explicit, the BinUtility construction now is.

Improved readability, this change has. But align the formatting better, we should.

Apply this formatting, you should:

-  binUtility += BinUtility{
-      BinningData(BinningOption::open, BinningValue::binX, 20, -10.0f, 10.0f)};
-  binUtility += BinUtility{
-      BinningData(BinningOption::open, BinningValue::binY, 20, -10.0f, 10.0f)};
+  binUtility += BinUtility{BinningData(
+      BinningOption::open, BinningValue::binX, 20, -10.0f, 10.0f)};
+  binUtility += BinUtility{BinningData(
+      BinningOption::open, BinningValue::binY, 20, -10.0f, 10.0f)};

Tests/UnitTests/Core/Geometry/ExtentTests.cpp (1)

189-209: Remove debug output and strengthen test coverage, we must!

Good start with basic roundtrip testing, this is. But improvements, suggest I do:

1. Remove debug output with std::cout, as temporary it seems
2. Add more test cases with different binning values, comprehensive coverage to achieve

Apply this diff to improve the test:

 BOOST_AUTO_TEST_CASE(JsonExtentRoundtripTests) {
   Extent e;
   e.set(BinningValue::binR, 0, 200);
   e.set(BinningValue::binZ, -50, 50);
 
   nlohmann::json j;
   j["extent"] = e;
 
-  std::cout << j.dump(2) << std::endl;
-
   Extent eIn = j["extent"];
 
   CHECK_CLOSE_ABS(eIn.min(BinningValue::binR), e.min(BinningValue::binR),
                   10e-5);
   CHECK_CLOSE_ABS(eIn.max(BinningValue::binR), e.max(BinningValue::binR),
                   10e-5);
   CHECK_CLOSE_ABS(eIn.min(BinningValue::binZ), e.min(BinningValue::binZ),
                   10e-5);
   CHECK_CLOSE_ABS(eIn.max(BinningValue::binZ), e.max(BinningValue::binZ),
                   10e-5);
+
+  // Test additional binning values
+  e.set(BinningValue::binPhi, -M_PI, M_PI);
+  e.set(BinningValue::binEta, -4.0, 4.0);
+  
+  j["extent"] = e;
+  eIn = j["extent"];
+  
+  CHECK_CLOSE_ABS(eIn.min(BinningValue::binPhi), e.min(BinningValue::binPhi),
+                  10e-5);
+  CHECK_CLOSE_ABS(eIn.max(BinningValue::binPhi), e.max(BinningValue::binPhi),
+                  10e-5);
+  CHECK_CLOSE_ABS(eIn.min(BinningValue::binEta), e.min(BinningValue::binEta),
+                  10e-5);
+  CHECK_CLOSE_ABS(eIn.max(BinningValue::binEta), e.max(BinningValue::binEta),
+                  10e-5);
 }

Core/src/Geometry/Extent.cpp (1)

215-240: Simplify the JSON serialization logic, young Padawan.

Unnecessary scope blocks, I sense. More elegant and concise, the code can be, hmm.

 void Acts::to_json(nlohmann::json& j, const Acts::Extent& e) {
-  {
     nlohmann::json jrange;
     const auto& xrange = e.range();
     for (auto ibv : allBinningValues()) {
       if (e.constrains(ibv)) {
         jrange[binningValueName(ibv)] = xrange[toUnderlying(ibv)];
       }
     }
     j["range"] = jrange;
-  }
-
-  {
     nlohmann::json jenvelope;
     const auto& envelope = e.envelope();
     for (auto ibv : allBinningValues()) {
       if (envelope[ibv] != zeroEnvelope) {
         jenvelope[binningValueName(ibv)] =
             Range1D<double>(envelope[ibv][0], envelope[ibv][1]);
       }
     }
     if (!jenvelope.empty()) {
       j["envelope"] = jenvelope;
     }
-  }
 }

Core/include/Acts/Geometry/Extent.hpp (1)

26-27: Forward declarations, a path to faster compilation they are.

Wise choice using json_fwd.hpp, reducing compilation dependencies it does. Clean and consistent with the Force, these JSON function declarations are.

Consider documenting the JSON format in the header file, help future Padawans it will.

Also applies to: 331-334

Tests/UnitTests/Plugins/Json/ProtoDetectorJsonConverterTests.cpp (2)

36-67: Hmmmm, equality comparison function, well-structured it is.

Thoroughly compare BinningData objects, this function does. But a small typo in variable name 'euqalStructure', I sense.

Fix the typo, we should:

-  bool euqalStructure =
+  bool equalStructure =
     (ba.subBinningData != nullptr)
         ? isEqual(*ba.subBinningData, *bb.subBinningData, tolerance)
         : (bb.subBinningData == nullptr);

-  BOOST_CHECK(euqalStructure);
+  BOOST_CHECK(equalStructure);

-  return equalBool && equalRange && euqalStructure;
+  return equalBool && equalRange && equalStructure;

---

69-88: Wise comparison of Extent objects, this is.

Handle floating-point comparisons with tolerance, you do. But early return on constraint mismatch, consider we should.

Optimize the flow, we can:

 bool isEqual(const Acts::Extent& ea, const Acts::Extent& eb,
              double tolerance = 0.) {
   bool equalConstrains = true;
   bool equalRange = true;
   for (auto& bVal : allBinningValues()) {
     equalConstrains =
         equalConstrains && (ea.constrains(bVal) == eb.constrains(bVal));
     BOOST_CHECK(equalConstrains);
+    if (!equalConstrains) {
+      return false;
+    }
     if (ea.constrains(bVal) && eb.constrains(bVal)) {
       equalRange =
           equalRange && std::abs(ea.min(bVal) - eb.min(bVal)) < tolerance;
       equalRange =
           equalRange && std::abs(ea.max(bVal) - eb.max(bVal)) < tolerance;
       BOOST_CHECK(equalRange);
+      if (!equalRange) {
+        return false;
+      }
     }
   }
-  BOOST_CHECK(equalConstrains);
-  BOOST_CHECK(equalRange);
   return equalRange && equalConstrains;
 }

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Check if nlohmann_json is properly set up in other CMake files

# Test: Search for nlohmann_json setup in parent CMake files
rg "find_package.*nlohmann_json" -g "CMakeLists.txt"

# Test: Check for any existing json-related options
rg "option.*json" -g "CMakeLists.txt"

#!/bin/bash
# Check if json is used in Core headers which would justify PUBLIC linkage
rg -l "nlohmann::json|nlohmann/json" --type hpp -g "Core/include/**"

# Check if json is only used in implementation files
rg -l "nlohmann::json|nlohmann/json" --type cpp -g "Core/src/**"

# Verify if json is properly set up in parent CMake before Core
fd "CMakeLists.txt" | xargs cat | awk '/add_subdirectory.*Core/{p=NR} /find_package.*nlohmann_json/{if(NR<p)print}'

#!/bin/bash
# Search for json usage in header files with correct extension
rg -l "nlohmann::json|nlohmann/json" -g "Core/include/**/*.h*"

# Check if any public headers include json headers
rg "#include.*json" -g "Core/include/**/*.h*"

# Check the order of find_package and add_subdirectory in root CMake
grep -E "find_package.*nlohmann_json|add_subdirectory.*Core" CMakeLists.txt

CMakeLists.txt (1)

313-320: Well structured, this implementation is!

But improve the error handling, we could. A message when falling back to bundled version, helpful it would be.

Apply this change, you should:

 if(ACTS_SETUP_NLOHMANN_JSON)
     if(ACTS_USE_SYSTEM_NLOHMANN_JSON)
         message(STATUS "Using system installation of nlohmann::json")
         find_package(nlohmann_json ${_acts_nlohmanjson_version} REQUIRED CONFIG)
     else()
+        message(STATUS "Using bundled nlohmann::json")
         add_subdirectory(thirdparty/nlohmann_json)
     endif()
 endif()

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Check if any parts of the codebase assume bundled nlohmann_json

# Search for direct includes of bundled json
rg -l "thirdparty/nlohmann_json"

# Search for json-related CMake conditions that might need updates
rg "ACTS_BUILD_PLUGIN_JSON|nlohmann_json"

#!/bin/bash
# Check for the new option and its usage
rg -A 5 "ACTS_USE_SYSTEM_NLOHMANN_JSON" CMakeLists.txt

# Check if there are any direct references to bundled json header paths
rg "thirdparty/nlohmann_json/.*\.hpp"

# Look for any potential json-related include paths
rg "#include.*json"

Core/src/Geometry/Extent.cpp (1)

215-240: Approve the serialization implementation, I do.

Clean and efficient, the code is. Serialize only what is needed, it does. Hmmmm.

Suggest extracting range and envelope serialization into separate functions, I do. Clearer intent and better testability, it would provide:

+void serializeRange(nlohmann::json& j, const Acts::Extent& e) {
+  nlohmann::json jrange;
+  const auto& xrange = e.range();
+  for (auto ibv : allBinningValues()) {
+    if (e.constrains(ibv)) {
+      jrange[binningValueName(ibv)] = xrange[toUnderlying(ibv)];
+    }
+  }
+  j["range"] = jrange;
+}
+
+void serializeEnvelope(nlohmann::json& j, const Acts::Extent& e) {
+  nlohmann::json jenvelope;
+  const auto& envelope = e.envelope();
+  for (auto ibv : allBinningValues()) {
+    if (envelope[ibv] != zeroEnvelope) {
+      jenvelope[binningValueName(ibv)] =
+          Range1D<double>(envelope[ibv][0], envelope[ibv][1]);
+    }
+  }
+  if (!jenvelope.empty()) {
+    j["envelope"] = jenvelope;
+  }
+}
+
 void Acts::to_json(nlohmann::json& j, const Acts::Extent& e) {
-  {
-    nlohmann::json jrange;
-    // ... range serialization
-  }
-  {
-    nlohmann::json jenvelope;
-    // ... envelope serialization
-  }
+  serializeRange(j, e);
+  serializeEnvelope(j, e);
 }

Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)

173-173: A typo in the documentation, there is.

"bollean" spelled incorrectly it is. "boolean" the correct spelling should be.

Apply this diff to fix the typo:

-/// @return a bollean if equal
+/// @return a boolean if equal

Configuration used: CodeRabbit UI
Review profile: CHILL

 void Acts::from_json(const nlohmann::json& j, Acts::Extent& e) {
+  if (!j.contains("range")) {
+    throw std::invalid_argument("Missing required 'range' field in JSON");
+  }
   const auto& jrange = j.at("range");
 
   for (const auto& [key, value] : jrange.items()) {
+    if (!value.is_object()) {
+      throw std::invalid_argument("Range values must be objects");
+    }
     BinningValue bval = binningValueFromName(key);
-    e.set(bval, value.at("min"), value.at("max"));
+    try {
+      const double min = value.at("min").get<double>();
+      const double max = value.at("max").get<double>();
+      e.set(bval, min, max);
+    } catch (const nlohmann::json::exception& ex) {
+      throw std::invalid_argument("Invalid min/max values in range");
+    }
   }
 
   if (j.contains("envelope")) {
     const auto& jenvelope = j.at("envelope");
+    if (!jenvelope.is_object()) {
+      throw std::invalid_argument("Envelope must be an object");
+    }
     ExtentEnvelope envelope;
 
     for (const auto& [key, value] : jenvelope.items()) {
+      if (!value.is_object()) {
+        throw std::invalid_argument("Envelope values must be objects");
+      }
       BinningValue bval = binningValueFromName(key);
-      envelope[bval] = {value["min"], value["max"]};
+      try {
+        const double min = value.at("min").get<double>();
+        const double max = value.at("max").get<double>();
+        envelope[bval] = {min, max};
+      } catch (const nlohmann::json::exception& ex) {
+        throw std::invalid_argument("Invalid min/max values in envelope");
+      }
     }
 
     e.setEnvelope(envelope);
   }
 }

-        std::runtime_error("Unsupported binning for Detray");
+        throw std::runtime_error("Unsupported binning for Detray");

-      std::runtime_error("Unsupported binning for Detray");
+      throw std::runtime_error("Unsupported binning for Detray");

    if (gridLink.contains(gridIndexType)) {
-      std::size_t& fGridIndex = gridLink[gridIndex];
+      std::size_t& fGridIndex = gridLink[gridIndexType];
      gridIndex = fGridIndex;
      fGridIndex++;

thirdparty/traccc/CMakeLists.txt (2)

18-23: Handle patch failures gracefully, you should!

Silent failures in the patch command, lead to the dark side they do. Consider adding error handling, wise it would be.

 FetchContent_Declare(
     traccc
     ${ACTS_TRACCC_SOURCE}
     PATCH_COMMAND
-        patch -p1 -i ${CMAKE_CURRENT_SOURCE_DIR}/remove-json-include.diff
+        bash -c "if ! patch -p1 --forward -i ${CMAKE_CURRENT_SOURCE_DIR}/remove-json-include.diff; then echo 'Patch failed to apply cleanly' >&2; exit 1; fi"
 )

---

18-23: Document the patch's purpose, you must!

Clear documentation, the path to understanding is. Add comments explaining the patch's purpose, help future maintainers it will.

+# Apply patch to remove JSON includes as part of making nlohmann::json a public dependency
 FetchContent_Declare(
     traccc
     ${ACTS_TRACCC_SOURCE}
     PATCH_COMMAND
         patch -p1 -i ${CMAKE_CURRENT_SOURCE_DIR}/remove-json-include.diff
 )

Core/include/Acts/Definitions/Algebra.hpp (1)

102-113: Strong with the Force, this design pattern is.

Wisely chosen, the ADL serializer pattern is. Through it:

• Automatic serialization in JSON containers, achieved it is
• Clean separation of concerns, maintained it remains
• Extension points for future types, provided they are

Yet, document these functions in your source files, you must. Clear documentation, the path to enlightenment is.

Tests/UnitTests/Core/Utilities/BinUtilityTests.cpp (1)

175-184: Ensure equality checks for all binning data, we must.

Although looping over binning data, the function is, not returning early if inequality found, it should. Consider adding a break or returning false immediately upon finding unequal binning data.

Apply this diff to improve the code:

   if (equal) {
     for (std::size_t ib = 0; ib < ba.binningData().size(); ++ib) {
       equal = isEqual(ba.binningData()[ib], bb.binningData()[ib], tolerance);
       BOOST_CHECK(equal);
+      if (!equal) {
+        return false;
+      }
     }
   }

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Verify patch file existence and content

# Check if patch file exists
if [ -f "thirdparty/traccc/remove-json-include.diff" ]; then
    echo "Found patch file"
    # Show patch content
    cat "thirdparty/traccc/remove-json-include.diff"
else
    echo "Patch file not found!"
fi

#!/bin/bash
# Search for the implementation of json serialization functions
echo "Searching for Transform3 JSON serialization implementation..."
rg "void Acts::to_json.*Transform3" --type cpp
rg "void Acts::from_json.*Transform3" --type cpp

#!/bin/bash
# Let us see the implementation details, hmm
echo "Implementation details, reveal we shall..."
rg "void Acts::to_json.*Transform3" -A 10 --type cpp
rg "void Acts::from_json.*Transform3" -A 10 --type cpp

#!/bin/bash
# Search for existing Transform3 JSON usage
echo "Searching for existing Transform3 JSON usage..."
rg "to_json.*Transform3|from_json.*Transform3" --type cpp

# Search for potential impact in traccc component
echo "Checking traccc component impact..."
rg "Acts/Plugins/Json/UtilitiesJsonConverter\.hpp"

#!/bin/bash
# Search for other JSON-related includes that might be affected
echo "Searching for JSON includes in the codebase..."
rg "include.*json" --type cpp

# Check for any other Transform3 serialization patterns
echo "Checking Transform3 serialization patterns..."
rg "Transform3.*serialize|serialize.*Transform3" --type cpp

# Look for implementation details
echo "Examining Transform3 JSON implementation..."
cat Core/src/Definitions/Algebra.cpp

 void Acts::from_json(const nlohmann::json& j, Acts::Extent& e) {
+  if (!j.contains("range")) {
+    throw std::invalid_argument("JSON object must contain 'range' field");
+  }
   const auto& jrange = j.at("range");

   for (const auto& [key, value] : jrange.items()) {
+    if (!value.contains("min") || !value.contains("max")) {
+      throw std::invalid_argument("Range must contain 'min' and 'max' fields");
+    }
     BinningValue bval = binningValueFromName(key);
     e.set(bval, value.at("min"), value.at("max"));
   }

+  if (j.contains("envelope")) {
     const auto& jenvelope = j.at("envelope");
     ExtentEnvelope envelope;

     for (const auto& [key, value] : jenvelope.items()) {
+      if (!value.contains("min") || !value.contains("max")) {
+        throw std::invalid_argument("Envelope must contain 'min' and 'max' fields");
+      }
       BinningValue bval = binningValueFromName(key);
       envelope[bval] = {value["min"], value["max"]};
     }

     e.setEnvelope(envelope);
   }
 }

   BOOST_CHECK(equalBoundaries);

-  return equalBool && equalRange && equalStructure;
+  return equalBool && equalRange && equalStructure && equalBoundaries;
 }

   BOOST_CHECK(equalRange);
-  bool euqalStructure =
+  bool equalStructure =
       (ba.subBinningData != nullptr)
           ? isEqual(*ba.subBinningData, *bb.subBinningData, tolerance)
           : (bb.subBinningData == nullptr);

-  BOOST_CHECK(euqalStructure);
+  BOOST_CHECK(equalStructure);

   BOOST_CHECK(equalBoundaries);

-  return equalBool && equalRange && euqalStructure;
+  return equalBool && equalRange && equalStructure && equalBoundaries;
 }